-
Notifications
You must be signed in to change notification settings - Fork 749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Adapter: Rise #2815
New Adapter: Rise #2815
Conversation
@misha93m PR checks are failing. Requesting you to please check. |
@misha93m PR is created as Draft. Let us know when PR is ready for review |
927a112
to
ebcc451
Compare
@misha93m Please let us know if this PR is ready to be reviewed. |
Hi @gargcreation1992 @onkarvhanumante |
adapters/rise/rise.go
Outdated
_ *adapters.ExtraRequestInfo) ( | ||
requestsToBidder []*adapters.RequestData, | ||
errs []error, | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick - for consistency in the codebase please use single line for MakeRequests function declaration.
func (a *adapter) MakeRequests(openRTBRequest *openrtb2.BidRequest, _ *adapters.ExtraRequestInfo) (requestsToBidder []*adapters.RequestData, errs []error) {
//your code
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Done.
adapters/rise/rise.go
Outdated
return "", errors.New("no publisherID supplied") | ||
} | ||
|
||
return publisherID, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick - you could combine the above if statement and simply return publisherID, errors.New("no publisherID supplied")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this solution will work. Due to your code, err
is initialised, and the caller will handle err
properly:
publisherID, err := a.extractPublisherID(openRTBRequest)
if err != nil {
errs = append(errs, fmt.Errorf("extractPublisherID: %w", err))
return nil, errs
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated code a bit. Please check.
adapters/rise/rise.go
Outdated
) | ||
|
||
for _, imp := range openRTBRequest.Imp { | ||
if err := json.Unmarshal(imp.Ext, &bidderExt); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should not ignore Unmarshal error here. Please do the error handling properly.
if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil {
return "", err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, updated.
adapters/rise/rise.go
Outdated
|
||
for _, imp := range openRTBRequest.Imp { | ||
if err := json.Unmarshal(imp.Ext, &bidderExt); err == nil { | ||
if err = json.Unmarshal(bidderExt.Bidder, &impExt); err == nil && impExt.PublisherID != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above. Please do not ignore Unmarshaling error here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, updated.
adapters/rise/rise.go
Outdated
func (a *adapter) MakeBids( | ||
request *openrtb2.BidRequest, | ||
_ *adapters.RequestData, | ||
responseData *adapters.ResponseData) (*adapters.BidderResponse, []error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting you to please move this to single line for consistency across the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, done.
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info.", responseData.StatusCode), | ||
} | ||
return nil, []error{err} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking response status code this way is correct but you could also make use of the following code -
if httputil.IsResponseStatusCodeNoContent(response) {
return nil, nil
}
if err := httputil.CheckResponseStatusCodeForErrors(response); err != nil {
return nil, []error{err}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, updated.
adapters/rise/rise.go
Outdated
return openrtb_ext.BidTypeBanner | ||
} else if imp.Video != nil { | ||
return openrtb_ext.BidTypeVideo | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning - This is an anti-pattern. This code assumes that for multi-format bid request, mediatype will be set to type banner by default. I would recommend to set the mediaType based on bidder response. Your bidder should be able to set openrtb2.Bid.Mtype field and use that field here to get mediaTypeForImp. Even if your bidder does not support multi-format request, I would highly encourage you to make this change. Here is an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, updated.
@@ -0,0 +1,18 @@ | |||
endpoint: "https://pbs.yellowblue.io/pbs" | |||
maintainer: | |||
email: rise-prog-dev@risecodes.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent an email confirmation to verify email addresss. Please reply received.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
email verified. thanks!
- video | ||
userSync: | ||
iframe: | ||
url: https://pbs-cs.yellowblue.io/pbs-iframe?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&redirect={{.RedirectURL}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User sync URL works. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting few changes.
Hi @gargcreation1992 @onkarvhanumante |
@misha93m tests are failing with following error:
|
return openrtb_ext.BidTypeAudio, nil | ||
case openrtb2.MarkupNative: | ||
return openrtb_ext.BidTypeNative, nil | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In rise.yaml, you have defined that your bidder will be supporting just Banner and Video. I believe you might wanna error out if bidder response returns Audio or Native as bid type. So here is the code -
switch bid.MType {
case openrtb2.MarkupBanner:
return openrtb_ext.BidTypeBanner, nil
case openrtb2.MarkupVideo:
return openrtb_ext.BidTypeVideo, nil
default:
return "", fmt.Errorf("unsupported MType %d", bid.MType)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, updated
No description provided.